Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #637, fix OS_ModuleUnload for static modules #638

Merged

Conversation

jphickey
Copy link
Contributor

@jphickey jphickey commented Nov 2, 2020

Describe the contribution
Ensure that the handle is not NULL before invoking dlclose(). In particular the handle will be NULL for static modules.

Fixes #637

Testing performed
Build and sanity test CFE with apps linked statically.

Expected behavior changes

  • Shutdown after CTRL+C occurs normally (no segfault)
  • Also confirmed that specifying an incorrect entry point symbol in startup script also benign. (because a bad entry point also causes module to be "unloaded" as part of cleanup, which triggered the bug too)

System(s) tested on
Ubuntu 20.04
i686-rtems4.11 / pc686 via QEMU

Additional context
This issue only affects statically linked apps, which "go through the motions" of module loading for consistency in operation, but do not actually load a module - because its already linked.

Contributor Info - All information REQUIRED for consideration of pull request
Joseph Hickey, Vantage Systems, Inc.

@jphickey jphickey added the CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) label Nov 2, 2020
@astrogeco astrogeco added CCB-20201104 and removed CCB:Ready Pull request is ready for discussion at the Configuration Control Board (CCB) labels Nov 4, 2020
Mark static modules with a different type, and check that type at unload.
Only call unload low level implementation if type is dynamic.
@astrogeco astrogeco requested a review from a user November 4, 2020 17:22
@astrogeco
Copy link
Contributor

CCB 2020-11-04 APPROVED

  • See documentation in cFE/cmake/sample_defs for doing a static build
    • Need to predeclare which symbols OS_Symbol_lookup will look for in sample_defs/targets.cmake

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. I like the support for static modules.

@astrogeco astrogeco changed the base branch from main to integration-candidate November 10, 2020 17:37
@astrogeco astrogeco merged commit f2f1f44 into nasa:integration-candidate Nov 10, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Nov 10, 2020
astrogeco added a commit to nasa/cFS that referenced this pull request Nov 16, 2020
* Add nasa/cFE#984

* Add nasa/cFE#980

* Add nasa/cFE#867

* Add nasa/osal#638 and update cfe due to rebase

* Add nasa/cFE#987

* Add nasa/to_lab#64 and nasa/sample_app#104

* Add nasa/osal#643

* Add nasa/cFE#1000

* Add nasa/ci_lab#58

* Add doxygen fixes for nasa/osal#643

* Add nasa/cFE#1013

* Add nasa/cFE#1011

* Add nasa/ci_lab#61

* Add nasa/sample_app#109

* Bump versions and point to submodules main

Co-authored-by: Joseph Hickey <[email protected]>
@jphickey jphickey deleted the fix-637-module-unload branch December 3, 2020 17:26
@skliper skliper added this to the 6.0.0 milestone Sep 24, 2021
jphickey pushed a commit to jphickey/osal that referenced this pull request Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OS_ModuleUnload() for statically loaded module may segfault
3 participants